Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Parse Relation not working #404

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix: Parse Relation not working #404

wants to merge 7 commits into from

Conversation

YBTopaz8
Copy link
Member

@YBTopaz8 YBTopaz8 commented Dec 21, 2024

Closes: #397

Did some general enhancements on the code structure it's more consistent (Working on object types are cool but a pain to debug since you never know the real type until you hit an exception.)

Did some general enhancements on the code structure it's more consistent (Working on `object` types are cool but a pain to debug since you never know the real type until you hit an exception.)

I tried to make it so the code is more readable as well.

Closes
#397
Copy link

parse-github-assistant bot commented Dec 21, 2024

Thanks for opening this pull request!

@YBTopaz8
Copy link
Member Author

YBTopaz8 commented Dec 21, 2024

this should be 2nd to be approved

@mtrezza mtrezza changed the title feat: Fixed Parse Relations not working fix: Parse Relations not working Dec 21, 2024
@mtrezza mtrezza changed the title fix: Parse Relations not working fix: Parse Relation not working Dec 21, 2024
@mtrezza
Copy link
Member

mtrezza commented Dec 21, 2024

You mentioned in the PR title only Parse Relations. But according to the description this PR closes 3 bugs; which one is true?

@YBTopaz8
Copy link
Member Author

YBTopaz8 commented Dec 21, 2024

@mtrezza they are all fixed !
Usernames are now returned as expected
And saving work on users, objects, relations - in fact all MutableObjects should all be saved properly.

This sample repo of mine showcases most if not all the basic/core Parse operations + having Parse Live Queries methods too for anyone in need of a Demo.

Those issues we caused by encoding, like with User, operations (and querying)

The demo is extremely simple.
If in need
download it,
in cmd run dotnet restore (might need to do dotnet workloads restore too) then run on your target platforms
All good 👍🏾

@mtrezza
Copy link
Member

mtrezza commented Dec 22, 2024

Nice, but how is it possible that the CI passes here even before we merged #403?

@YBTopaz8
Copy link
Member Author

Because Like I said, the main issues were very like likely linked to either encoding or the way we received Https responses.

So fixing "Encoding" likely provided a stable foundation for all Https Enc/Decoding.

That's my thought process at least.

@YBTopaz8
Copy link
Member Author

So they likely pass because Relations didn't have methods implemented AND didn't have test implemented to test coverage.

@YBTopaz8
Copy link
Member Author

In anticipation to any other questions let me fully explain the changes I made...;

  1. Changes All tests especially the latest ones as they lacked proper ini and and clean up. (Some test descriptions we added here and there
  2. Parse Relations were broken due to no update to the encoding in order to support relations. As they now inherited from IParseFieldOperation which is JSON convertible, I had to ensure operations are converted and de-converted as needed.
  3. ParseCommand, Data which gets the command data, encodes in Stream to POST/PUT had an issue where the operation was only done once and out of sync compared to its context. So I had to put a null coalescing operator as an check that should usually ensure we have the object with its data on demand.
  4. There are some other changes I made like breaking down 1 line into 2 because writing one line of code having over 4 methods chained together is never helpful especially after pouring hours on understanding the code base.

These are the highlights. any other change was just to ensure field operations work as they should.

@mtrezza
Copy link
Member

mtrezza commented Dec 24, 2024

Could you please take a look at the conflicts so we can review this? Seems to be just minor formatting.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've mentioned in #404 (comment) this PR contains a lot of unrelated formatting changes like indentation, spaces, newline. This makes the PR more difficult to review. Some of these changes are likely done manually, other may be done automatically by your IDE. Since we don't have a linter yet, we'll make an exception and ignore these changes in this PR, so we can go ahead faster. However, to make PRs easier to review, could you please make sure that a PR doesn't contain such changes? Just take a look at the changes before you the commit. If you see any unnecessary line changes, please revert them before committing. You can also see these changes on GitHub in the "Files changed" tab. I've opened #406 to add a linter, which will prevent these changes to get merged in the future, but it's still in the works.

@YBTopaz8
Copy link
Member Author

Sure thing. I'll be more diligent on that aspect.

Adding the linter will help indeed, as sometimes the file changes don't even reflect on my end.

But all is good now.

Next PR should be easier to Navigate through

@mtrezza
Copy link
Member

mtrezza commented Dec 26, 2024

Thank you. Before I merge, could you please suggest a title for this PR that describes all the things that this PR fixes? The #397 seems quite broad as it mentions 3 problems in #397 (comment) and in #397 (comment) you mention that it "clears your issues and any other potential one" so this PR fixes more than the 3 issues? If yes, what are these other issues? Or do all these issues have a common, underlying bug that has been fixed? This is quite confusing.

@YBTopaz8
Copy link
Member Author

Sure.
The title is quite fitting you may add (+ bug fixes)
As I

  1. Fixed .SaveAsync() which didn't work due to encoder/decoder not accounting for all possible data types we support
  2. ParseClient.Instance.GetCurrentUser(); now returns all the expected data including User name (was missing before)
  3. All Relations-Related issues are fixed (Get, Save, Remove)
  4. ParseClient.Instance.Logout() can now be called on its own, without async nor cancellation token. If in need to pass a token, we can still call await ParseClient.Instance.LogoutAsync(). The aim was to avoid doing everything asynchronously
  5. Changed from IsAuthenticated Property to a method IsAuthenticatedAsync() both returning a bool, but the latter was needed to correct .SaveAsync()
  6. Misc change: renamed LoginAsync() to LoginWithAsync() and SignUpAsync() to SignUpWithAsync(). Just a name change as per comments left in the file - and it makes it more intuitive so I did that too.
  7. Changes done to UWC : redid my approach as explained already - ensuring Data transfer is posted and gotten correctly.

That's pretty much it. everything else is unchanged (unless I haven't caught it/if it's an issue!)

Just ONE change I want to do later and it will be to REPLACE this below. It doesn't negatively affect At all. but it will always print out "Unsupported length" all the time as an exception. I have the fix already but will hold on to that, perhaps in the next PR - depends on you I suppose.

    try
        {
            totalLength = responseStream.Length;
        }
        catch
        {
            Console.WriteLine("Unsupported length...");
        };

@mtrezza
Copy link
Member

mtrezza commented Dec 27, 2024

Are all these points 1-7 related to the bug with Parse Relations? Or are these actually 7 individual bugs? Or are some of these not bugs but new features or perhaps performance optimizations? And are any of these points breaking changes?

@YBTopaz8
Copy link
Member Author

YBTopaz8 commented Dec 27, 2024

  • All of these are ultimately related to Encoding/Decoding due to v4.0.0's implementation being incomplete at the time.
    Point 7 is an adjacent change aimed at making code more readable. All of the changes in Point 7 can be reverted if need.

  • None of the bugs here are new since Parse supported the features before.

  • None of these are aimed optimizing performance..

  • For breaking Changes.. Well on top of more tests, I Created a repo, now online, where I tested the core Parse Functionalities login, save file, CRUD parse objects and Relations, call cloud code function, and much more! And they all work and are reproducible to/by all.

  • Any bug occurring would be an interesting one to me as well and I'll be sure to look into

@mtrezza
Copy link
Member

mtrezza commented Dec 27, 2024

I don't follow. I was asking whether these points are related to the issue with Parse Relation and you say they are all related to encoding/decoding. So how is encoding/decoding related to the bug with Parse Relation?

I also don't understand: "None of the bugs here are new since Parse supported the features before." What features are you referring to? So this PR does fix other bugs besides the Parse Relation bug?

For example, how are points 1 or 4 related to Parse Relation, they sound like completely different bugs?

Point 6 sounds like a breaking change since it's a name change of a public method? And how is that required to fix the Parse Relation bug? Note that the only accepted way to demonstrate that something does not break is a unit test that passes before and after the change. If that unit test doesn't exist already, it needs to be added as part of the PR.

If you prefer, I believe we may resolve all these questions quicker in a chat. Feel free to join our community chat and ping me there.

@mtrezza
Copy link
Member

mtrezza commented Jan 30, 2025

@YBTopaz8 How's it going with this PR? What do you thin is needed to merge it, or parts of it? Would it help to split it up into smaller PRs maybe? Would be good if we could get his merged, so we can merge #396 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing methods in Parse .NET SDK 4.0
2 participants